AI Assistant skill menu: one mixed chooser for cards + skill files#5547
Conversation
Preview deploymentsHost Test Results200 tests 200 ✅ 3m 25s ⏱️ Results for commit c5f03f0. Realm Server Test Results 1 files ±0 1 suites ±0 16m 9s ⏱️ -41s Results for commit c5f03f0. ± Comparison against earlier commit 064e678. |
28eab3f to
cd62452
Compare
Collapse the two skill-attach buttons into a single action that opens the
mixed cards + files chooser, and retire the file-tree path for skills.
- One `Choose a Skill to add` button opens `chooseCard({ includeFiles: true })`
with a base filter that spans both kinds: `any` of the Skill card type and
MarkdownDef files whose `kind` is `skill`. Each pick is routed by its kind —
a card to `onChooseCard`, a file to `onChooseSkillMarkdown`.
- Already-attached skills are excluded by `not: { eq: { id } }` clauses built
client-side from the menu's own skill list: card skills are excluded
immediately, and file skills once their rows carry `id` in the search doc
(Phase 1 reindex), so this ships without waiting on a deployed reindex.
- The separate skill-file button and its `chooseFile`-based attach are removed.
`chooseFile` / `FileChooserModal` stay for their other consumers (message
file attach, Playground, markdown-embed).
Tests: the single button attaches a skill card and a skill markdown file;
already-attached card and markdown skills are excluded; the retired skill-file
button is gone; canceling the chooser re-enables the add button.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The skill picker opens the mixed chooser (`includeFiles`), which renders a tile per matching row. With no realm scope it fanned out across every server realm — including large shared realms — so the picker stalled on rendering and the intended skills may never appear. Pass `realms: userRealmIdentifiers` so the picker searches only the user's own workspaces, where the skills they attach live. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lies The skill menu scopes its mixed chooser to `realmServer.userRealmIdentifiers`. Under mock matrix that list stayed empty (only `activeRealms` was set), so the scope was a no-op and the chooser fanned out across every server realm — including the large shared skills realm — and the intended skill tiles never rendered in time. Set `activeRealmServers` in the mock-matrix setup (as the workspace-chooser integration test does) so the user realm list populates with the test realm, exercising the same scoped path the skill menu takes in production. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
d88beea to
e10622e
Compare
…lected
The skill menu's base filter is `any: [{ type: skillCardRef }, { on:
markdownDefRef, eq: { kind: 'skill' } }]`. When the type picker seeds a
selected type, `buildSearchQuery` stripped the base filter's type constraint —
but `stripTypeFromFilter` collapsed the `any` (dropping the `skillCardRef`
branch) down to the lone `{ on, eq }` markdown branch, which then AND-ed into
the top-level `every`. Every row was required to be a `kind: 'skill'` markdown
file, so skill *cards* were excluded and only markdown skills rendered.
Only strip a simple single-type base filter (which is all the optimization was
meant for); keep a compound base filter intact so its alternation survives.
Same guard applied to `buildRecentsQuery`.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…helper
Two review fixes:
- The client-built already-attached exclusions were `not: { eq: { id } }`,
which drop any row whose `id` is absent (`NOT (NULL = X)` is NULL under
three-valued logic). On a realm not yet carrying the stamped file `id`, that
hid *every* skill-file row whenever any skill was attached. Wrap each in
`any: [{ eq: { id: null } }, { not: { eq: { id } } }]` so a null id keeps the
row while a present-and-matching id still excludes it.
- Move the `any`-collapse guard into `stripTypeFromFilter` itself: never strip
within an `any` (dropping/collapsing a branch changes which rows match), so
the invariant holds for any caller. Drops the band-aid `isCardTypeFilter`
guards at the two call sites.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Attachable skills live mostly in the shared Boxel Skills realm, which the user-realms-only scope excluded, so they were not selectable. Scope the mixed chooser to the user's own workspaces plus `skillsRealmURL`. Kept bounded (rather than searching every server realm) so the federated search does not stall on unrelated realms' contents. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The skill chooser now searches the shared Boxel Skills realm as well as the user's workspaces. In the e2e env that realm is indexed from scratch on server startup, so the chooser's federated search stalls until it is ready and the skill tests time out. Add a module-scoped `beforeAll` that waits (polling a known public skill card) until the realm is served from its index before the tests run. Scoped to this module so unrelated matrix suites don't pay the wait. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The previous warm-up polled a single skill card, which is served before the
Boxel Skills realm finishes indexing — so the chooser's federated search still
stalled on the mid-index realm and the tests timed out. Poll `/_queue-status`
(monitoring-gated; bearer token = sha256('MONITORING' + realm-server seed))
until `pending: 0`, i.e. the whole realm-server index queue has drained, before
this module's tests run.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Root cause of the matrix skills failures (verified from the harness source, not inferred): `support/isolated-realm-server.ts` marks `:4205/test/` and `:4205/skills/` as catalog realms (`show_as_catalog = true`), and the skill fixtures (e.g. `skill-pirate-speak`) live in `:4205/test/`. The scope was `userRealmIdentifiers + skillsRealmURL`, which excludes catalog realms — so the fixtures were never searched and the chooser showed nothing. The federated search itself is fast (200 in ~100-200ms); indexing was never the issue (the harness already gates tests on `_readiness-check`), so the queue-drain warm-up is reverted. Scope the chooser to user workspaces + catalog realms + the Boxel Skills realm. Includes a temporary `[SKILL-SCOPE]` console.warn to confirm the resolved scope in CI; will be removed once green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Confirmed from the failing run's Playwright trace: the catalog-scope fix works (the chooser shows "79 results across 2 realms" — Boxel Skills + Test Workspace A), but the shared Boxel Skills realm contributes 76 skills that render first, pushing the target test-realm skill below the fold — so its tile exists but is not visible and the click times out. Have `attachSkill` type the target's URL into the chooser search to narrow to it before clicking (the same pattern messages.spec.ts already uses). Also drop the temporary scope-diagnostic console.warn. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Root cause of the blank/unclickable skill tiles (confirmed live via DevTools): the chooser set `initialFocusedSection` to the first selected realm, and `isCollapsed` collapses every *other* realm's section to just its header (`display:none` on its grid). Once the skill chooser scoped to multiple realms (user + catalog + skills), the target skill's realm — not the first — was collapsed, so its tile rendered at height 0 and the click timed out. (The old chooser passed no realm scope, so nothing was focused or collapsed.) Only auto-focus when the scope is a single realm (where collapsing is a no-op); for a multi-realm scope, focus nothing so every section's results stay visible. Verified in the running app: the target tile now renders at 252px, clickable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖]
Reviewed with a focus on the mixed-chooser correctness path — filter construction (the exclusions plus the base any), the kind-routing of a pick, and the two shared-component edits (stripTypeFromFilter, SearchPanel focus) this feature drags in — since those are where a single skill-menu change can ripple into other choosers.
Bottom line: no blocking code issues. The prior review threads are all correctly resolved (I re-verified each against the code), and the design is sound. The one thing gating merge is CI, which is red on the head commit.
What lands right
- Collapsing the two attach buttons into one
chooseCard(query, { includeFiles: true })and routing bychosen.kindis clean; both branches already funnel to the sameattachSkillTask, so there's no consumer churn.chosen.idis the right value for each kind — the modal deliberately skips.json-stripping forfilepicks, so a markdown file's id stays its source URL, which is whatattachSkillTaskwants. - The NULL-safe exclusion (
any: [{ eq: { id: null } }, { not: { eq: { id } } }]) is the correct three-valued-logic fix, and shipping ahead of a deployed reindex is a reasonable call given the degradation is harmless (see the inline note on the exclusion). - Moving the
any-preservation intostripTypeFromFilteritself, rather than guarding at the two call sites, is the right home for the invariant, and the new unit test pins it.
Re-verification of the resolved threads (all correctly closed)
- NULL-safe exclusions — correct; the markdown case is verified via
RoomSkill.cardId === sourceUrl(inline). - Scope now user + catalog + skills — and because
skillsRealmURLis always in the set, the earlier "emptyuserRealmIdentifiersre-opens the fan-out" edge case is closed in practice (therealmsarray is never empty), which settles that deferred thread too. any-collapse moved into the helper — correct, with regression coverage.
Recommendations
- Get CI green before merge (blocking for merge, not a code defect). The head commit shows a host-test error (shard 13 — the aggregate's
1 🔥/1 ❌) plus a Host Memory Baseline failure. I could not attribute the shard-13 error to a specific test: testem's per-test output lands in the junit artifact, not job stdout, and the artifact download was blocked in my environment. Please re-run shard 13 to establish flaky vs. real; the memory-baseline job is typically informational (a test was added here, which can shift the baseline). - Optional: tighten the
buildSearchQuerycomment aboutstripTypeFromFilter(inline note).
Adjacent, out of scope: the pre-existing "skill picker excludes already-enabled skills" test fills the search field with 'Exanple' (a typo for "Example") — not touched by this PR and evidently still passing, but worth a fix by whoever next edits that file.
Generated by Claude Code
e0d1789
into
cs-12204-file-id-unification
Closes CS-12206. Phase 3 (the user-facing payoff) of the mixed cards+files chooser (parent CS-11809). Stacked on #5546 (CS-12205, mixed chooser) → #5531 (CS-12204, file
id). Review/merge those first.What
One button in the AI Assistant skill menu now attaches either a Skill card or a skill-bearing markdown file, from a single search chooser.
attachSkillCard+attachSkillMarkdowninto one action that opens the mixed chooser viachooseCard(query, { includeFiles: true }). The base filter spans both kinds —anyof{ type: skillCardRef }and{ on: markdownDefRef, eq: { kind: 'skill' } }.onChooseCard(id), a file pick →onChooseSkillMarkdown(id). Both already flow to the sameattachSkillTaskinmatrix/room.gts, so no consumer change.not: { eq: { id } }clauses built client-side from the menu's own skill list. Card skills are excluded immediately; file skills once their rows carryidin the search doc (Phase 1 reindex) — so the feature ships without waiting on a deployed reindex.chooseFile-based attach are removed.chooseFile/FileChooserModalstay for their other consumers (message file attach, Playground, markdown-embed) — untouched.Tests (
skills-test.gts)lint:types(host), eslint, and ember-template-lint are green.Note
Depends on #5546 and #5531. In CI the index is fresh (Phase 1
idstamping active), so the markdown-skill exclusion is exercised end-to-end. In a deployed environment, file-skill exclusion becomes effective after the affected realms are reindexed; card-skill exclusion needs no reindex.🤖 Generated with Claude Code